Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: New Apis for Snap #60

Merged
merged 14 commits into from
Sep 23, 2024
Merged

feat: New Apis for Snap #60

merged 14 commits into from
Sep 23, 2024

Conversation

ShubhamParkhi
Copy link
Contributor

This PR updates the dependencies required to update the Apis, implement homepage and lifecycle hooks

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for kleros-contract-insights-snap ready!

Name Link
🔨 Latest commit 572e57d
🔍 Latest deploy log https://app.netlify.com/sites/kleros-contract-insights-snap/deploys/66f17d864ab0cb00081bbe7d
😎 Deploy Preview https://deploy-preview-60--kleros-contract-insights-snap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@gmkung
Copy link
Collaborator

gmkung commented Sep 18, 2024

@ShubhamParkhi just tested it. Looks good, just a few comments:

  1. The deeplink works well!
  2. For the onInstall page, I'm suggesting to change "Domain: Which domain(s) is this contract interacted in?" to "Domain: Whether this contract is known to be used on this domain"
  3. For the original CDN result, I'm thinking we can add custom logic for blockchain explorers, such that if domain is one of these, we don't show the deeplink message, as users may accidentally submit the suggest and get challenged.
  4. For the onUpdate hook, currently I see that the text is empty. Is it meant to show something?

@gmkung
Copy link
Collaborator

gmkung commented Sep 20, 2024

Tested again and the fixes above are good. @ShubhamParkhi
one last thing: i think now the suggestion to submit the CDN entry is shown when no insights are available but I think we can make it it's also shown when it's not one of the explorer domains AND if the CDN insight is not present (rather than only when no insights are present).

@ShubhamParkhi ShubhamParkhi marked this pull request as ready for review September 23, 2024 08:45
gmkung
gmkung previously approved these changes Sep 23, 2024
Copy link
Collaborator

@gmkung gmkung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested all these changes last Friday. LGTM.

jaybuidl
jaybuidl previously approved these changes Sep 23, 2024
Copy link

sonarcloud bot commented Sep 23, 2024

@jaybuidl jaybuidl merged commit 8c579a5 into kleros:master Sep 23, 2024
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants